Skip to content

Conversation

@Prashanth684
Copy link
Contributor

@Prashanth684 Prashanth684 commented Nov 10, 2025

split the camgi part out of #133 to make it easier to review and more readable

Summary by CodeRabbit

  • New Features

    • Added CAMGI (Cluster Autoscaler Must‑Gather Inspector) to must-gather — start/stop via /must-gather:camgi [must-gather-path|stop], launches an interactive web UI for autoscaler and cluster infrastructure analysis; supports local or containerized execution, auto-browser opening, and permission handling.
  • Documentation

    • Added comprehensive CAMGI docs, quick start, examples, troubleshooting, usage tips, and integration guidance.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 10, 2025

Walkthrough

Adds CAMGI support to the must-gather plugin: a new /must-gather:camgi command entry, three documentation pages (README-CAMGI, command doc, SKILL updates), and a new executable launcher script run-camgi.sh to start/stop CAMGI via local binary, Python module, or container runtime.

Changes

Cohort / File(s) Summary
Plugin metadata
docs/data.json
Add camgi command entry to must-gather plugin with name, description, synopsis /must-gather:camgi [must-gather-path], and argument_hint `[must-gather-path
Plugin manifest docs
PLUGINS.md
Document new CAMGI command in plugin manifest (documentation-only addition).
CAMGI documentation
plugins/must-gather/README-CAMGI.md, plugins/must-gather/commands/camgi.md
Add CAMGI-focused docs: overview, quick start, start/stop usage, installation options (container/pip/local), container flags and SELinux notes, troubleshooting, included files, examples referencing the launcher script, and usage synopsis.
Analyzer skill docs
plugins/must-gather/skills/must-gather-analyzer/SKILL.md
Integrate CAMGI into analyzer guidance: usage notes, interpretation tips, output format, helper script references, CAMGI-specific analysis guidance, and next-step recommendations.
Launcher script
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh
New executable script implementing CAMGI orchestration: arg parsing (must-gather-path or stop), path discovery/validation, permission checks with optional auto-fix, stop logic for existing containers, runtime fallback (local binary → python -m → podman/docker), container invocation flags, port/defaults (8080), and optional browser auto-open.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Script as run-camgi.sh
    participant Local as okd-camgi (binary/module)
    participant Container as Podman/Docker
    participant CAMGI as CAMGI Service
    participant Browser

    User->>Script: run-camgi.sh [must-gather-path | stop]
    Script->>Script: parse args & validate path/layout
    alt stop
        Script->>Container: locate & stop CAMGI containers (image filter)
        Script-->>User: stopped / status
    else start
        Script->>Script: check file ownership/permissions
        alt needs fix
            Script->>User: prompt to fix permissions (auto-fix option)
            User-->>Script: approve/deny
        end

        rect rgba(220,235,255,0.9)
        note right of Script: Execution fallback chain
        Script->>Local: attempt local binary / python -m okd_camgi
        Local-->>Script: available / not found
        alt not available
            Script->>Container: run container (podman/docker) with mounts, port, SELinux relabel
            Container->>CAMGI: start service (127.0.0.1:8080)
            CAMGI-->>Script: ready
        end
        end

        Script->>Browser: open 127.0.0.1:8080 (if possible)
        Script-->>User: report URL, PID/container status, and next steps
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Focus review on:
    • plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (arg parsing, fallback order, container run flags, SELinux relabeling, permission-fix flow, stop logic).
    • docs/data.json entry consistency with plugin command schema.
    • Cross-document consistency of synopsis, argument hints, port/defaults, and user-facing messages across PLUGINS.md, commands/camgi.md, README-CAMGI.md, SKILL.md, and the script.

Pre-merge checks and finishing touches

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Git Push Safety Rules ❓ Inconclusive Repository could not be accessed to verify code changes for git operations or automated workflows. Provide direct repository access or file contents, particularly run-camgi.sh script and GitHub Actions workflows.
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a skill to launch CAMGI for must-gather analysis, which is reflected across multiple files including the new CAMGI command, documentation, and launch script.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
No Real People Names In Style References ✅ Passed PR summary review shows no real people's names used as style references in CAMGI functionality documentation or implementation files.
No Assumed Git Remote Names ✅ Passed All six modified files checked for hardcoded git remote names like 'origin' or 'upstream'. No instances found in any files including run-camgi.sh script.
No Untrusted Mcp Servers ✅ Passed Pull request adds CAMGI launch functionality through documentation, configuration, and shell scripts without introducing MCP servers or untrusted external package dependencies.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (6)
plugins/must-gather/README-CAMGI.md (2)

10-13: Add language identifiers to fenced code blocks.

All code blocks should specify the language for proper syntax highlighting. Add bash identifiers to the bash code blocks.

Examples:

- ```
+ ```bash
  ./run-camgi.sh /path/to/must-gather
- ```
+ ```

Apply this consistently to all bash blocks throughout the file (lines 10–13, 37–40, 54–56, 116–123, 136–140, 170–172).

Also applies to: 37-40, 54-56, 116-123, 136-140, 170-172


75-76: Wrap bare URLs in markdown link syntax.

Several URLs are bare and should be wrapped in markdown link syntax for consistency and better rendering.

Examples:

- - CAMGI GitHub: https://github.com/elmiko/okd-camgi
+ - CAMGI GitHub: [https://github.com/elmiko/okd-camgi](https://github.com/elmiko/okd-camgi)

Apply to lines 75–76, 106, and 160.

Also applies to: 106-106, 160-160

plugins/must-gather/commands/camgi.md (2)

47-47: Grammar: Fix verb form in file permissions description.

Lines 47 and 190 should use the infinitive form "to read" instead of the noun "read".

Apply this diff:

-Must-gather files need read permissions for the container user.
+Must-gather files need to read permissions for the container user.

Actually, the better phrasing would be:

-Must-gather files need read permissions for the container user.
+Must-gather files need read access for the container user.

Or:

-Must-gather files need read permissions for the container user.
+The container user needs read permissions for must-gather files.

Also applies to: 190-190


58-58: Minor grammar improvements.

  • Line 58: Add missing comma after closing parenthesis: "...which should not contain secrets**,**"
  • Line 105: Change "local install" to "local installation" for proper noun form

Also applies to: 105-105

plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (2)

35-35: Shellcheck SC2155: Separate variable declaration and assignment.

Store the command output in a separate step to avoid masking return values and improve clarity.

Apply to lines 35 and 50:

- local containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)
+ local containers
+ containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)

Also applies to: 50-50


180-180: Browser opening via xdg-open may silently fail in non-graphical environments.

Lines 180 and 194 attempt to open the browser via xdg-open, but this tool is not available in headless systems, SSH sessions, or container environments. The error is silently suppressed (2>/dev/null), leaving the user uncertain whether the browser opened.

Consider:

  1. Checking if xdg-open is available before attempting to use it
  2. Providing clearer messaging to the user that the browser opening was skipped
  3. Suggesting the user manually navigate to the URL if browser opening fails

This is a UX refinement; the functionality still works (server starts regardless), but users on headless systems won't know whether the browser opening succeeded.

Also applies to: 194-194

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between cc69412 and 9e8da09.

📒 Files selected for processing (6)
  • PLUGINS.md (1 hunks)
  • docs/data.json (1 hunks)
  • plugins/must-gather/README-CAMGI.md (1 hunks)
  • plugins/must-gather/commands/camgi.md (1 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/SKILL.md (3 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/commands/camgi.md

[grammar] ~47-~47: The verb “read” needs to be in the to-infinitive form.
Context: ...File Permissions:* Must-gather files need read permissions for the container user. The...

(MISSING_TO_BEFORE_A_VERB)


[uncategorized] ~58-~58: Possible missing comma found.
Context: ... -R a+r`). This is safe for must-gather data which should not contain secrets. **Ma...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~74-~74: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...

(NOT_ABLE_PREMIUM)


[uncategorized] ~93-~93: Possible missing preposition found.
Context: ...provided as argument, ask the user - Accept either root must-gather directory or ex...

(AI_HYDRA_LEO_MISSING_TO)


[grammar] ~105-~105: The word ‘install’ is not a noun.
Context: ...erized version (podman/docker) or local install - Open browser automatically at http...

(A_INSTALL)


[grammar] ~190-~190: The verb “read” needs to be in the to-infinitive form.
Context: ...File Permissions: Must-gather files need read permissions for the container user - **...

(MISSING_TO_BEFORE_A_VERB)

🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/README-CAMGI.md

10-10: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


37-37: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


54-54: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


75-75: Bare URL used

(MD034, no-bare-urls)


76-76: Bare URL used

(MD034, no-bare-urls)


106-106: Bare URL used

(MD034, no-bare-urls)


116-116: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


136-136: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


160-160: Bare URL used

(MD034, no-bare-urls)


170-170: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh

[warning] 35-35: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 50-50: Declare and assign separately to avoid masking return values.

(SC2155)

🔇 Additional comments (2)
PLUGINS.md (1)

116-116: LGTM! Documentation addition is clear, well-placed, and follows the existing format.

plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)

151-215: Strong implementation of multiple fallback methods with verified permission checking.

The run_camgi() function demonstrates good defensive programming with clear fallback paths (local binary → Python module → podman → docker) and helpful error messages guiding users through installation.

Permission checking logic (lines 116–141) verified:

  • The find ... ! -perm -004 command correctly identifies files without world-read permission
  • The chmod -R a+r command safely uses the default behavior (does not follow symlinks by default; uses -P implicitly)
  • Error suppression via 2>/dev/null is acceptable here since the script provides fallback messaging (lines 131–133) guiding users to manually fix permissions if needed
  • Edge cases with special files (sockets, FIFOs) are unlikely to block most must-gather archives

The logic is sound and handles missing dependencies gracefully.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
docs/data.json (1)

545-550: Inconsistency between synopsis and argument_hint (existing feedback).

The synopsis shows only [must-gather-path] but argument_hint includes the stop option. These should be consistent.

Apply this diff to fix:

-        "synopsis": "/must-gather:camgi [must-gather-path]",
+        "synopsis": "/must-gather:camgi [must-gather-path|stop]",
🧹 Nitpick comments (2)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (2)

30-45: Fix Shellcheck SC2155 warning: declare and assign separately.

Line 35 assigns to containers within a pipeline, which masks the return value of podman ps. Split the declaration and assignment to properly capture the exit code.

  stop_camgi() {
      echo -e "${BLUE}Stopping camgi containers...${NC}"

      # Check for podman
      if command_exists podman; then
-         local containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)
+         local containers
+         containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)
          if [ -n "$containers" ]; then

48-61: Fix Shellcheck SC2155 warning: declare and assign separately (docker check).

Line 50 has the same issue as line 35. Split the declaration and assignment for docker.

      # Check for docker
      if command_exists docker; then
-         local containers=$(docker ps -q --filter ancestor=quay.io/elmiko/okd-camgi)
+         local containers
+         containers=$(docker ps -q --filter ancestor=quay.io/elmiko/okd-camgi)
          if [ -n "$containers" ]; then
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9e8da09 and 6ac0b31.

📒 Files selected for processing (6)
  • PLUGINS.md (1 hunks)
  • docs/data.json (1 hunks)
  • plugins/must-gather/README-CAMGI.md (1 hunks)
  • plugins/must-gather/commands/camgi.md (1 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/SKILL.md (3 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • PLUGINS.md
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/commands/camgi.md

[grammar] ~47-~47: The verb “read” needs to be in the to-infinitive form.
Context: ...File Permissions:* Must-gather files need read permissions for the container user. The...

(MISSING_TO_BEFORE_A_VERB)


[uncategorized] ~58-~58: Possible missing comma found.
Context: ... -R a+r`). This is safe for must-gather data which should not contain secrets. **Ma...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~74-~74: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...

(NOT_ABLE_PREMIUM)


[uncategorized] ~93-~93: Possible missing preposition found.
Context: ...provided as argument, ask the user - Accept either root must-gather directory or ex...

(AI_HYDRA_LEO_MISSING_TO)


[grammar] ~105-~105: The word ‘install’ is not a noun.
Context: ...erized version (podman/docker) or local install - Open browser automatically at http...

(A_INSTALL)


[grammar] ~190-~190: The verb “read” needs to be in the to-infinitive form.
Context: ...File Permissions: Must-gather files need read permissions for the container user - **...

(MISSING_TO_BEFORE_A_VERB)

🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/commands/camgi.md

21-21: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3

(MD001, heading-increment)

plugins/must-gather/README-CAMGI.md

21-21: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3

(MD001, heading-increment)

🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh

[warning] 35-35: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 50-50: Declare and assign separately to avoid masking return values.

(SC2155)

🔇 Additional comments (6)
plugins/must-gather/README-CAMGI.md (1)

1-173: Clear documentation for CAMGI integration and setup.

The README provides comprehensive coverage of CAMGI features, prerequisites, troubleshooting, and technical details. The structure with Quick Start, Commands, Features, Installation, and Troubleshooting sections makes this easy to follow.

plugins/must-gather/skills/must-gather-analyzer/SKILL.md (3)

179-200: CAMGI section well-integrated into storage analysis.

The new CAMGI section fits naturally after storage analysis and before the interpret-and-report phase. The documentation clearly explains when and how to use CAMGI for autoscaler investigation, with proper references to detailed docs.


261-265: CAMGI launcher script properly documented in helper scripts reference.

The entry concisely documents the launcher script's purpose, output, prerequisites, and reference to detailed documentation.


293-296: New "Cluster autoscaler problems" workflow adds practical guidance.

The new scenario flow (run CAMGI → inspect UI → review decisions) provides clear next steps for users troubleshooting autoscaler issues.

plugins/must-gather/commands/camgi.md (1)

1-202: Well-structured command documentation with clear implementation details.

The documentation comprehensively covers prerequisites, error handling, implementation flow, and practical examples. The connection to the launcher script at plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh is clearly documented. Users have clear guidance on permissions, port conflicts, SELinux, and browser issues.

plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)

150-215: Well-implemented CAMGI launcher with robust fallback chain.

The run_camgi() function methodically attempts multiple approaches (local binary → Python module → containerized) with clear error messaging at each stage. The use of --webbrowser flag for local execution and automatic browser opening for containers provides good UX. Permissions checking with interactive fixing is user-friendly.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2025
@openshift-ci
Copy link

openshift-ci bot commented Nov 24, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Prashanth684

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
PLUGINS.md (1)

1-150: Address pipeline lint failure: Run make update to regenerate PLUGINS.md.

The CI pipeline reports that PLUGINS.md is out of sync with plugin metadata. This must be resolved before the PR can merge.

Run make update to regenerate the file with the correct metadata, then commit the updated PLUGINS.md.

♻️ Duplicate comments (1)
docs/data.json (1)

669-674: Unresolved: Update synopsis to include stop option.

The synopsis field shows [must-gather-path] but argument_hint includes the stop option. These must be consistent.

Apply this diff to align synopsis with argument_hint:

-        "synopsis": "/must-gather:camgi [must-gather-path]",
+        "synopsis": "/must-gather:camgi [must-gather-path|stop]",

Note: The pipeline also reports docs/data.json is out of sync with plugin metadata. Run make update to regenerate both this file and PLUGINS.md, then verify this correction is applied.

🧹 Nitpick comments (3)
plugins/must-gather/commands/camgi.md (2)

27-27: Add language identifier to code fence.

The code fence on line 27 should specify bash as the language for proper syntax highlighting.

Apply this diff:

-## Synopsis
-```
-/must-gather:camgi [must-gather-path]
-/must-gather:camgi stop
-```
+## Synopsis
+```bash
+/must-gather:camgi [must-gather-path]
+/must-gather:camgi stop
+```

35-35: Format bare URLs according to Markdown best practices.

Several bare URLs should be wrapped in inline code blocks or markdown links for consistency with linting standards (MD034).

Examples of URLs to format:

  • Line 35: https://github.com/elmiko/okd-camgi`https://github.com/elmiko/okd-camgi`
  • Line 67: http://127.0.0.1:8080`http://127.0.0.1:8080`
  • Line 139, 153, 175-177: Similar formatting

This improves consistency with project markdown standards.

Also applies to: 67-67, 139-139, 153-153, 175-177

plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)

35-35: Improve error handling: Separate variable declaration and assignment (SC2155).

The current pattern masks return values from the command substitution. Declare and assign separately to properly detect command failures.

Apply this diff to both instances:

-    local containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)
+    local containers
+    containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)
     if [ -n "$containers" ]; then

Repeat the same pattern for line 50 with docker ps.

This ensures that if podman ps or docker ps fails, the script can detect and handle the error instead of silently continuing with an empty result.

Also applies to: 50-50

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 6ac0b31 and aeeb121.

📒 Files selected for processing (6)
  • PLUGINS.md (1 hunks)
  • docs/data.json (1 hunks)
  • plugins/must-gather/README-CAMGI.md (1 hunks)
  • plugins/must-gather/commands/camgi.md (1 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/SKILL.md (3 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • plugins/must-gather/README-CAMGI.md
🧰 Additional context used
🪛 GitHub Actions: Lint Plugins
PLUGINS.md

[error] 1-1: PLUGINS.md is out of sync with plugin metadata. Run 'make update' to update.

docs/data.json

[error] 1-1: docs/data.json is out of sync with plugin metadata. Run 'make update' to update.

🪛 LanguageTool
plugins/must-gather/commands/camgi.md

[style] ~79-~79: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...

(NOT_ABLE_PREMIUM)

🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/commands/camgi.md

27-27: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


35-35: Bare URL used

(MD034, no-bare-urls)


67-67: Bare URL used

(MD034, no-bare-urls)


139-139: Bare URL used

(MD034, no-bare-urls)


153-153: Bare URL used

(MD034, no-bare-urls)


175-175: Bare URL used

(MD034, no-bare-urls)


176-176: Bare URL used

(MD034, no-bare-urls)


177-177: Bare URL used

(MD034, no-bare-urls)

🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh

[warning] 35-35: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 50-50: Declare and assign separately to avoid masking return values.

(SC2155)

🔇 Additional comments (3)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md (1)

192-213: LGTM! CAMGI documentation well integrated.

The CAMGI sections are properly documented with usage instructions, helper script reference, and workflow examples. The integration with existing analysis guidance is clear.

Also applies to: 274-278, 306-309

plugins/must-gather/commands/camgi.md (1)

2-2: Description appropriately scopes CAMGI's multi-purpose use.

The description and detailed capabilities list (lines 20–28) properly convey CAMGI's broader applications beyond autoscaler analysis, including cluster operators, Machines, MachineSets, and infrastructure inspection, addressing the past observation about CAMGI's versatility.

Also applies to: 15-29

plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)

1-241: LGTM! Well-structured launcher script with good error handling and fallback strategies.

The script demonstrates solid shell scripting practices:

  • Multiple methods for running CAMGI (local binary → Python module → containers)
  • Intelligent must-gather directory auto-detection (lines 97–109)
  • Permission checking with user-interactive fixes (lines 112–141)
  • Proper use of color output for UX
  • SELinux-aware volume mounting with :Z flag (lines 183, 196)
  • Clear error messages with remediation guidance (lines 200–214)

The only improvement needed is addressing the SC2155 shellcheck warnings noted in the separate comment above.

Please confirm that the script has been tested with both podman and docker, and that the browser launching with xdg-open works in your target environments.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
docs/data.json (1)

669-674: Inconsistency between synopsis and argument_hint (duplicate issue).

The synopsis shows only [must-gather-path] but argument_hint includes both [must-gather-path|stop]. For consistency and clarity, the synopsis should reflect both available options.

Apply this diff:

-        "synopsis": "/must-gather:camgi [must-gather-path]",
+        "synopsis": "/must-gather:camgi [must-gather-path|stop]",
🧹 Nitpick comments (1)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)

35-35: Fix shellcheck SC2155 violations: declare and assign separately.

Combining local declaration with command substitution masks return values from podman and docker commands. If either command fails, the error is silently hidden.

Apply this diff to fix both occurrences:

- stop_camgi() {
-     echo -e "${BLUE}Stopping camgi containers...${NC}"
-
-     # Check for podman
-     if command_exists podman; then
-         local containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)
+     local containers
+     containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)

Apply the same pattern at line 50 for the docker command.

Also applies to: 50-50

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between aeeb121 and eab8848.

📒 Files selected for processing (6)
  • PLUGINS.md (1 hunks)
  • docs/data.json (1 hunks)
  • plugins/must-gather/README-CAMGI.md (1 hunks)
  • plugins/must-gather/commands/camgi.md (1 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/SKILL.md (3 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • PLUGINS.md
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/commands/camgi.md

[style] ~79-~79: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...

(NOT_ABLE_PREMIUM)

🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md

216-216: Bare URL used

(MD034, no-bare-urls)


281-281: Bare URL used

(MD034, no-bare-urls)

plugins/must-gather/commands/camgi.md

10-10: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


42-42: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


59-59: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


80-80: Bare URL used

(MD034, no-bare-urls)


81-81: Bare URL used

(MD034, no-bare-urls)


111-111: Bare URL used

(MD034, no-bare-urls)


121-121: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


146-146: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


170-170: Bare URL used

(MD034, no-bare-urls)


180-180: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


186-186: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


192-192: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


208-208: Bare URL used

(MD034, no-bare-urls)


209-209: Bare URL used

(MD034, no-bare-urls)

plugins/must-gather/README-CAMGI.md

27-27: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


35-35: Bare URL used

(MD034, no-bare-urls)


67-67: Bare URL used

(MD034, no-bare-urls)


139-139: Bare URL used

(MD034, no-bare-urls)


153-153: Bare URL used

(MD034, no-bare-urls)


175-175: Bare URL used

(MD034, no-bare-urls)


176-176: Bare URL used

(MD034, no-bare-urls)


177-177: Bare URL used

(MD034, no-bare-urls)

🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh

[warning] 35-35: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 50-50: Declare and assign separately to avoid masking return values.

(SC2155)

🔇 Additional comments (2)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)

1-241: Script implementation looks solid overall.

The error handling, user guidance, and fallback strategy are well-designed. The auto-detection of must-gather subdirectory structure (lines 98–109) and interactive permission fixing (lines 112–141) provide good user experience. Helpful error messages and multiple launch methods make the script robust.

Once the SC2155 warnings are addressed, this is ready.

plugins/must-gather/skills/must-gather-analyzer/SKILL.md (1)

192-217: CAMGI documentation additions are well-integrated.

The CAMGI section (lines 192–217) provides comprehensive usage details, the helper scripts reference (lines 279–284) is properly formatted, and the new scenario (lines 312–316) fits naturally into the existing structure. The duplicate section from the previous review has been resolved.

Also applies to: 279-284, 312-316

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

♻️ Duplicate comments (1)
docs/data.json (1)

669-674: The synopsis is still inconsistent with argument_hint; include the stop option.

The synopsis shows only [must-gather-path] but argument_hint includes the stop option. Based on the command documentation and usage, the synopsis should reflect both available options.

Apply this diff to fix:

-        "synopsis": "/must-gather:camgi [must-gather-path]",
+        "synopsis": "/must-gather:camgi [must-gather-path|stop]",
🧹 Nitpick comments (1)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)

30-64: Duplicated container stop logic across podman and docker checks.

The stop_camgi() function repeats the same logic for podman and docker. Consider refactoring to reduce duplication:

stop_camgi_runtime() {
    local runtime="$1"
    local containers=$($runtime ps -q --filter ancestor=quay.io/elmiko/okd-camgi 2>/dev/null)
    if [ -n "$containers" ]; then
        echo "$containers" | while read container; do
            echo -e "${BLUE}Stopping container: $container${NC}"
            $runtime stop "$container"
        done
        echo -e "${GREEN}Stopped camgi container(s)${NC}"
    fi
}

# Then in stop_camgi():
for runtime in podman docker; do
    if command_exists "$runtime"; then
        stop_camgi_runtime "$runtime"
    fi
done

This reduces the ~35 lines of duplication to a single loop. However, the current implementation is functional and the duplication may be acceptable for maintainability.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between eab8848 and 7e45466.

📒 Files selected for processing (6)
  • PLUGINS.md (1 hunks)
  • docs/data.json (1 hunks)
  • plugins/must-gather/README-CAMGI.md (1 hunks)
  • plugins/must-gather/commands/camgi.md (1 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/SKILL.md (3 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • plugins/must-gather/README-CAMGI.md
  • PLUGINS.md
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/commands/camgi.md

[style] ~79-~79: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...

(NOT_ABLE_PREMIUM)

🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/commands/camgi.md

27-27: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


35-35: Bare URL used

(MD034, no-bare-urls)


67-67: Bare URL used

(MD034, no-bare-urls)


139-139: Bare URL used

(MD034, no-bare-urls)


153-153: Bare URL used

(MD034, no-bare-urls)


175-175: Bare URL used

(MD034, no-bare-urls)


176-176: Bare URL used

(MD034, no-bare-urls)


177-177: Bare URL used

(MD034, no-bare-urls)

plugins/must-gather/skills/must-gather-analyzer/SKILL.md

216-216: Bare URL used

(MD034, no-bare-urls)


281-281: Bare URL used

(MD034, no-bare-urls)

🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh

[warning] 35-35: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 50-50: Declare and assign separately to avoid masking return values.

(SC2155)

🔇 Additional comments (6)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md (1)

312-316: Good addition to common scenarios.

The new scenario section for cluster infrastructure/autoscaler problems provides clear guidance for users encountering these issues. The workflow is consistent with other scenarios and appropriately references the CAMGI tool.

plugins/must-gather/commands/camgi.md (1)

1-216: Comprehensive and well-structured command documentation.

The documentation provides excellent coverage of CAMGI functionality, prerequisites, error handling, implementation details, and usage examples. The progressive disclosure of information (from simple usage to technical details) follows good documentation practices. The error handling section is particularly valuable for user experience.

plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (4)

173-198: Browser auto-launch may fail silently on some systems; ensure fallback is clear.

The script attempts to launch the browser using xdg-open in the background at lines 180 and 194. While the fallback messaging is good (lines 175-176), xdg-open may not exist or work on all systems (e.g., headless servers, non-Linux). The silent failure (with 2>/dev/null) is appropriate, but consider adding a note in output about manual URL access.

Current behavior is acceptable given the existing fallback guidance, but document this design decision.


151-215: Well-structured multi-method startup strategy with good error handling.

The run_camgi() function implements a sensible priority order:

  1. Local binary (fastest)
  2. Python module (good fallback)
  3. Containerized (works when local not available)

Each method includes appropriate feedback, and the final error message is helpful with actionable install instructions. The integration with slash commands via documented scripts is appropriate.


92-109: Robust must-gather path detection with helpful fallback.

The path validation includes auto-detection of the subdirectory structure (lines 98-109), which is user-friendly. The search pattern for *sha256* is appropriate for OpenShift must-gather format. Good defensive programming.


111-141: Interactive permission fixing is user-friendly and safe.

The permission check at lines 111-141 is well-implemented:

  • Detects restrictive permissions
  • Prompts user interactively
  • Provides fallback if user declines
  • Clear guidance on manual fix

One note: The find command at line 116 with ! -perm -004 checks for files without world-read. For must-gather data (which should not contain secrets), this is appropriate. Good defensive coding.

@theobarberbany
Copy link
Contributor

@Prashanth684 do we want to address the coderabbit review?

Also, does this make more sense as a command? Why have you chosen to package this as a skill?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
docs/data.json (1)

669-674: Inconsistent synopsis and argument_hint for camgi command.

The synopsis should reflect all available options. It currently only shows [must-gather-path], but argument_hint includes the stop option. Update the synopsis for consistency:

  {
    "name": "camgi",
    "description": "Launch CAMGI (Cluster Autoscaler Must-Gather Inspector) web interface to analyze cluster infrastructure and autoscaler behavior",
-   "synopsis": "/must-gather:camgi [must-gather-path]",
+   "synopsis": "/must-gather:camgi [must-gather-path|stop]",
    "argument_hint": "[must-gather-path|stop]"
  }
🧹 Nitpick comments (6)
plugins/must-gather/commands/camgi.md (6)

10-13: Add language specifier to synopsis code block (line 10).

Markdown linting requires language specifiers for fenced code blocks. The synopsis block should use bash since it shows shell command syntax:

  ## Synopsis
- ```
+ ```bash
  /must-gather:camgi [must-gather-path]
  /must-gather:camgi stop
- ```
+ ```

42-48: Add language specifier to must-gather directory structure block (line 42).

Add text as the language specifier for the directory listing:

  Must-gather data with autoscaler information:
- ```
+ ```text
  must-gather/
  └── registry-ci-openshift-org-origin-...-sha256-<hash>/
      ├── cluster-scoped-resources/
      ├── namespaces/
      └── ...
- ```
+ ```

59-61: Add language specifier to permission fix command block (line 59).

  If container shows permission denied errors, the script will prompt:
- ```
+ ```bash
  Fix permissions now? (Y/n)
- ```
+ ```

121-143: Add language specifier to success output example block (line 121).

The output block should use text as the language specifier:

  **On Successful Launch:**
- ```
+ ```text
  CAMGI is now running!
  
  The web interface should open automatically in your browser at:
  http://127.0.0.1:8080
  
  ... (rest of output)
- ```
+ ```

146-149: Add language specifier to stop output example block (line 146).

  **On Stop:**
- ```
+ ```text
  Stopping all CAMGI containers...
  CAMGI stopped successfully.
- ```
+ ```

179-195: Add language specifiers to example command blocks (lines 180, 186, 192).

The example blocks showing command invocations should include bash as the language specifier:

  1. **Start CAMGI with path**:
- ```
+ ```bash
   /must-gather:camgi /home/user/must-gather

Launches CAMGI with the provided must-gather path and opens browser.

  1. Start CAMGI interactively:
  •  /must-gather:camgi

    Asks for must-gather path, then launches CAMGI.

    1. Stop CAMGI:
  •  /must-gather:camgi stop
    Stops all running CAMGI containers.

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used**: Path: .coderabbit.yaml

**Review profile**: CHILL

**Plan**: Pro

**Cache: Disabled due to data retention organization setting**

**Knowledge base: Disabled due to data retention organization setting**

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 7e454666710729a6977e02ff44f0031cb37b3fc0 and 59656c07acd65254c84d0b55c38dab8fcd28b58a.

</details>

<details>
<summary>📒 Files selected for processing (6)</summary>

* `PLUGINS.md` (1 hunks)
* `docs/data.json` (1 hunks)
* `plugins/must-gather/README-CAMGI.md` (1 hunks)
* `plugins/must-gather/commands/camgi.md` (1 hunks)
* `plugins/must-gather/skills/must-gather-analyzer/SKILL.md` (3 hunks)
* `plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh` (1 hunks)

</details>

<details>
<summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary>

* plugins/must-gather/README-CAMGI.md
* PLUGINS.md

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>🪛 LanguageTool</summary>

<details>
<summary>plugins/must-gather/commands/camgi.md</summary>

[style] ~79-~79: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:**  If the web interface is not accessible: - The script opens [http://127.0.0.1:8...

(NOT_ABLE_PREMIUM)

</details>

</details>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>

<details>
<summary>plugins/must-gather/commands/camgi.md</summary>

10-10: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

42-42: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

59-59: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

121-121: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

146-146: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

170-170: Bare URL used

(MD034, no-bare-urls)

---

180-180: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

186-186: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

192-192: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</details>

</details>
<details>
<summary>🪛 Shellcheck (0.11.0)</summary>

<details>
<summary>plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh</summary>

[error] 36-36: Remove spaces around = to assign (or use [ ] to compare, or quote '=' if literal).

(SC2283)

---

[warning] 36-36: Quote this to prevent word splitting.

(SC2046)

</details>

</details>

</details>

<details>
<summary>🔇 Additional comments (1)</summary><blockquote>

<details>
<summary>plugins/must-gather/skills/must-gather-analyzer/SKILL.md (1)</summary><blockquote>

`192-217`: **✓ CAMGI documentation well integrated.**

The CAMGI section is well-documented with proper markdown formatting, wrapped URLs, and clear examples. The integration with other analysis methods is seamless.

</blockquote></details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2025
@Prashanth684 Prashanth684 force-pushed the ai-mg-camgi branch 2 times, most recently from 2d6015f to dcfd27e Compare November 25, 2025 21:41
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 59656c0 and dcfd27e.

📒 Files selected for processing (6)
  • PLUGINS.md (1 hunks)
  • docs/data.json (1 hunks)
  • plugins/must-gather/README-CAMGI.md (1 hunks)
  • plugins/must-gather/commands/camgi.md (1 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/SKILL.md (3 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • PLUGINS.md
  • plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh
  • docs/data.json
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/commands/camgi.md

[style] ~79-~79: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens [http://127.0.0.1:8...

(NOT_ABLE_PREMIUM)

🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/README-CAMGI.md

27-27: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


35-35: Bare URL used

(MD034, no-bare-urls)


67-67: Bare URL used

(MD034, no-bare-urls)


139-139: Bare URL used

(MD034, no-bare-urls)


153-153: Bare URL used

(MD034, no-bare-urls)


175-175: Bare URL used

(MD034, no-bare-urls)


176-176: Bare URL used

(MD034, no-bare-urls)


177-177: Bare URL used

(MD034, no-bare-urls)

🔇 Additional comments (1)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md (1)

192-217: Unable to verify CAMGI duplication due to repository access issues.

The verification scripts cannot execute because the repository is not accessible in the current environment. To complete this verification, I would need to:

  1. Count occurrences of #### CAMGI headers in the full SKILL.md file
  2. Confirm whether the file contains exactly one CAMGI section
  3. Verify that lines 201-222 (where the duplicate was reportedly located) no longer contain duplicate content

Please manually verify by running:

grep -n "#### CAMGI" plugins/must-gather/skills/must-gather-analyzer/SKILL.md

This will show all CAMGI section locations. There should be exactly one match if the duplicate has been removed.

1. Detect must-gather subdirectory structure
2. **Check and fix file permissions** (with your confirmation)
3. Start camgi (containerized or local install)
4. Open web interface at http://127.0.0.1:8080
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Wrap bare URLs in markdown link syntax.

Per markdownlint (MD034), bare URLs must be wrapped in markdown link syntax. Fix these instances:

- 4. Open web interface at http://127.0.0.1:8080
+ 4. Open web interface at [http://127.0.0.1:8080](http://127.0.0.1:8080)
  
- **Automatic Browser Opening**
- Opens http://127.0.0.1:8080 automatically
+ **Automatic Browser Opening**
+ Opens [http://127.0.0.1:8080](http://127.0.0.1:8080) automatically

Also wrap these URLs (lines 139, 153, 175–177):

  • Line 139: http://127.0.0.1:8080[http://127.0.0.1:8080](http://127.0.0.1:8080)
  • Line 153: http://127.0.0.1:9090[http://127.0.0.1:9090](http://127.0.0.1:9090)
  • Line 175: https://github.com/elmiko/okd-camgi[https://github.com/elmiko/okd-camgi](https://github.com/elmiko/okd-camgi)
  • Line 176: https://pypi.org/project/okd-camgi/[https://pypi.org/project/okd-camgi/](https://pypi.org/project/okd-camgi/)
  • Line 177: https://docs.openshift.com/...[https://docs.openshift.com/...](https://docs.openshift.com/...)

Also applies to: 67-67, 139-139, 153-153, 175-177

🧰 Tools
🪛 markdownlint-cli2 (0.18.1)

35-35: Bare URL used

(MD034, no-bare-urls)

🤖 Prompt for AI Agents
plugins/must-gather/README-CAMGI.md lines 35, 67, 139, 153, 175-177: several
bare URLs violate markdownlint MD034; wrap each bare URL in markdown link syntax
(e.g. change http://127.0.0.1:8080 to
[http://127.0.0.1:8080](http://127.0.0.1:8080), http://127.0.0.1:9090 to
[http://127.0.0.1:9090](http://127.0.0.1:9090),
https://github.com/elmiko/okd-camgi to
[https://github.com/elmiko/okd-camgi](https://github.com/elmiko/okd-camgi),
https://pypi.org/project/okd-camgi/ to
[https://pypi.org/project/okd-camgi/](https://pypi.org/project/okd-camgi/), and
the OpenShift docs URL to its equivalent [URL](URL) form) so all bare links are
enclosed in [text](url) markdown.

split the camgi part out of openshift-eng#133 to make it easier to review and more readable
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
docs/data.json (1)

688-693: I'm unable to access the repository directly due to cloning failures in the sandbox environment. However, I can verify the review comment based on the evidence provided within it.

The review comment includes the actual code snippet showing:

  • Lines 688-693: The camgi command entry with argument_hint: "[must-gather-path|stop]" and synopsis: "/must-gather:camgi [must-gather-path]"

The inconsistency is clear and verifiable from the provided snippet itself:

  • The argument_hint explicitly includes two options: must-gather-path and stop
  • The synopsis only shows must-gather-path, omitting the stop option

This is a legitimate inconsistency. When a command supports multiple argument options (indicated by the pipe | in argument_hint), both should be reflected in the synopsis for accurate documentation.

The review comment is well-founded, the suggested fix is appropriate, and the concern about this being previously flagged but not fixed is valid.


The synopsis field for the camgi command must include the stop option to match argument_hint.

The current inconsistency shows argument_hint includes [must-gather-path|stop] but synopsis only shows [must-gather-path]. Apply the suggested fix:

         {
           "argument_hint": "[must-gather-path|stop]",
           "description": "Launch CAMGI (Cluster Autoscaler Must-Gather Inspector) web interface to analyze cluster infrastructure and autoscaler behavior",
           "name": "camgi",
-          "synopsis": "/must-gather:camgi [must-gather-path]"
+          "synopsis": "/must-gather:camgi [must-gather-path|stop]"
         }
🧹 Nitpick comments (2)
plugins/must-gather/commands/camgi.md (1)

10-13: Add language specifiers to fenced code blocks (markdown MD040).

Several code blocks lack language specifiers required by markdown linting. Apply these fixes:

 ## Synopsis
-```
+```bash
 /must-gather:camgi [must-gather-path]
 /must-gather:camgi stop
-```
+```bash
 Must-gather data with autoscaler information:
-```
+```text
 must-gather/
 └── registry-ci-openshift-org-origin-...-sha256-<hash>/
     ├── cluster-scoped-resources/
     ├── namespaces/
     └── ...
-```
+```text
 **Manual fix** (if needed):
-```bash
+```bash
 chmod -R a+r /path/to/must-gather
-```
+```bash

For the output/message blocks (lines 121–143, 146–149), use text:

 **On Successful Launch:**
-```
+```text
 CAMGI is now running!
 ...
-```
+```text
 **On Stop:**
-```
+```text
 Stopping all CAMGI containers...
 CAMGI stopped successfully.
-```
+```text

Similarly, update example command blocks (lines 180–195) to use bash:

 1. **Start CAMGI with path**:
-   ```
+   ```bash
    /must-gather:camgi /home/user/must-gather
-   ```
+   ```bash
 2. **Start CAMGI interactively**:
-   ```
+   ```bash
    /must-gather:camgi
-   ```
+   ```bash
 3. **Stop CAMGI**:
-   ```
+   ```bash
    /must-gather:camgi stop
-   ```
+   ```bash

Also applies to: 42-48, 59-62, 121-143, 146-149, 180-195

plugins/must-gather/README-CAMGI.md (1)

27-29: Fix markdown linting issues: add language specifier and wrap bare URLs.

Apply these fixes for markdown compliance:

  1. Add bash language specifier to code block (line 27):
 Or use the slash command from anywhere:
-```
+```bash
 /must-gather:camgi /path/to/must-gather
-```
+```bash
  1. Wrap bare URLs in markdown link syntax (lines 139, 153, 175–177):
 **Solution:** Use http://127.0.0.1:8080 instead of localhost
+ **Solution:** Use [http://127.0.0.1:8080](http://127.0.0.1:8080) instead of localhost
 Then access at http://127.0.0.1:9090
+ Then access at [http://127.0.0.1:9090](http://127.0.0.1:9090)
 ## References
 
-- CAMGI GitHub: https://github.com/elmiko/okd-camgi
-- CAMGI on PyPI: https://pypi.org/project/okd-camgi/
-- Must-Gather Documentation: https://docs.openshift.com/container-platform/latest/support/gathering-cluster-data.html
+- CAMGI GitHub: [https://github.com/elmiko/okd-camgi](https://github.com/elmiko/okd-camgi)
+- CAMGI on PyPI: [https://pypi.org/project/okd-camgi/](https://pypi.org/project/okd-camgi/)
+- Must-Gather Documentation: [https://docs.openshift.com/container-platform/latest/support/gathering-cluster-data.html](https://docs.openshift.com/container-platform/latest/support/gathering-cluster-data.html)

Also applies to: 139-139, 153-153, 175-177

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between dcfd27e and 2af3479.

📒 Files selected for processing (6)
  • PLUGINS.md (1 hunks)
  • docs/data.json (1 hunks)
  • plugins/must-gather/README-CAMGI.md (1 hunks)
  • plugins/must-gather/commands/camgi.md (1 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/SKILL.md (3 hunks)
  • plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • plugins/must-gather/skills/must-gather-analyzer/SKILL.md
  • plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh
  • PLUGINS.md
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/commands/camgi.md

[style] ~79-~79: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens [http://127.0.0.1:8...

(NOT_ABLE_PREMIUM)

🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/README-CAMGI.md

27-27: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


139-139: Bare URL used

(MD034, no-bare-urls)


153-153: Bare URL used

(MD034, no-bare-urls)


175-175: Bare URL used

(MD034, no-bare-urls)


176-176: Bare URL used

(MD034, no-bare-urls)


177-177: Bare URL used

(MD034, no-bare-urls)

plugins/must-gather/commands/camgi.md

10-10: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


42-42: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


59-59: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


121-121: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


146-146: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


180-180: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


186-186: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


192-192: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

@Prashanth684
Copy link
Contributor Author

@Prashanth684 do we want to address the coderabbit review?

Also, does this make more sense as a command? Why have you chosen to package this as a skill?

addressed the comments. The reason for making it a skill was to be more prescriptive and deterministic when deploying camgi, i.e consistently use the same image - deploy the containers in the same fashion rather than leaving it up to the LLM to decide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants